Fix #13068: make PluginDependenciesResolver methods default - #13069
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to restoring binary compatibility, uses safe delegation consistent with existing DefaultPluginDependenciesResolver behavior, and is protected by both unit and integration regression tests.
Pull request overview
This PR restores binary compatibility for out-of-tree PluginDependenciesResolver implementations by changing the two methods introduced as abstract in 4.0.0-rc-6 (resolveCoreExtensionAndFlatten, resolvePluginAndFlatten) into default methods that delegate to the pre-existing resolvePlugin, preventing AbstractMethodError in IDE embedders compiled against older Maven versions.
Changes:
- Make
PluginDependenciesResolver.resolveCoreExtensionAndFlatten(...)andresolvePluginAndFlatten(...)defaultmethods delegating toresolvePlugin(...), with@implSpecguidance to avoid recursion and clarify best-effort behavior. - Add unit coverage to lock in the “must remain default methods” contract and verify delegation behavior.
- Add an integration test that compiles a legacy resolver against
maven-core:4.0.0-rc-5and verifies it runs cleanly on newer Maven via a core extension.
File summaries
| File | Description |
|---|---|
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/PluginDependenciesResolver.java |
Turns the two newly-added abstract methods into default methods delegating to resolvePlugin to preserve binary compatibility. |
impl/maven-core/src/test/java/org/apache/maven/plugin/internal/PluginDependenciesResolverDefaultMethodsTest.java |
Verifies delegation and asserts the methods are truly default (guards against regression). |
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh13068LegacyPluginDependenciesResolverTest.java |
End-to-end regression test proving legacy compiled resolver works without AbstractMethodError. |
its/core-it-suite/src/test/java/org/apache/maven/it/TestSuiteOrdering.java |
Ensures the new integration test is included/ordered in the core IT suite. |
its/core-it-suite/src/test/resources/gh-13068-legacy-plugin-dependencies-resolver/** |
Adds the test fixture projects (legacy extension + client using .mvn/extensions.xml). |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
resolveCoreExtensionAndFlatten and resolvePluginAndFlatten were added as abstract methods in 4.0.0-rc-6, forward-porting the Maven 3.10.0 changes (apache#12335). The interface is documented as internal, but it is the only hook Maven offers for influencing plugin resolution, and every major Java IDE overrides it: IntelliJ IDEA, Eclipse m2e and NetBeans. Those implementations live out of tree and are compiled against one Maven while running on another, so adding abstract methods turns plugin resolution into an AbstractMethodError. Both methods now default to the pre-existing resolvePlugin, which DefaultPluginDependenciesResolver already treats as an alias -- there, resolvePlugin delegates to resolvePluginAndFlatten. resolvePlugin itself deliberately stays abstract: a default there would let an implementation overriding neither method recurse infinitely. The default for resolveCoreExtensionAndFlatten is best effort. A dedicated implementation additionally reads the extension's artifact descriptor to apply relocations and run MavenPluginDependenciesValidator. Verified against the class shipped in IntelliJ IDEA 2026.2.2, which implements resolvePluginAndFlatten but not resolveCoreExtensionAndFlatten and therefore still fails on stock rc-6. Adds a unit test pinning the compatibility contract and a core IT that builds an extension against maven-core 4.0.0-rc-5 -- the last release before the methods existed -- and runs a build through it. Verified by Christofer Dutz (@chrisdutz) on Apache PLC4X: a build carrying this change imports cleanly in IntelliJ IDEA, where stock rc-6 fails. Closes apache#13068 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W2HnhKt7MJWYsSSVUmPrvU
e191d1b to
d90273d
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The fix is correct and the test coverage is exemplary.
ASK (change-type signals detected):
- New
defaultmethods on a public interface — binary compat contract, delegation safety - Delegation cycle risk — mutual delegation between
resolvePlugin↔resolvePluginAndFlatten - Test adequacy — does the IT fixture actually exercise the default method path, or does it accidentally trigger a full override?
NARROW / READ / DECIDE:
Default method delegation chain — non-issue. The chain from BootstrapCoreExtensionManager through a legacy impl is: resolveCoreExtensionAndFlatten default → resolvePlugin (legacy impl overrides, delegates to DefaultPluginDependenciesResolver.resolvePlugin) → resolvePluginAndFlatten (in DefaultPluginDependenciesResolver, not the interface default) → resolveInternal. No recursion. null artifact is handled at line 255 of DefaultPluginDependenciesResolver. The DefaultMavenPluginManager calls resolvePluginAndFlatten directly — for a legacy impl that path hits the interface default → resolvePlugin → legacy impl → delegate. Also clean.
Delegation cycle warning — non-issue. The warning on resolvePlugin ("must not delegate to resolvePluginAndFlatten without also overriding it") is correct. An impl that does delegate there would recurse, and the warning catches it. The warning in the <p> tag on a @Deprecated method is slightly easy to miss, but the PR body documents the intent clearly and DefaultPluginDependenciesResolver sets the example.
Best-effort resolveCoreExtensionAndFlatten default — non-issue. The default skips the artifact descriptor read (relocations, MavenPluginDependenciesValidator). This is acknowledged in the Javadoc and confirmed real-world by @chrisdutz — IntelliJ IDEA 2026.2.2 works with this. The tradeoff (silent downgrade vs. UnsupportedOperationException) favors leniency for a compat shim.
IT fixture integrity — non-issue. The LegacyPluginDependenciesResolver implements resolvePlugin (which logs the marker line), not the newer methods. When DefaultMavenPluginManager calls resolvePluginAndFlatten on it, the call routes through the interface default → resolvePlugin → legacy impl logs the marker. The verifyTextInLog assertion on "[gh-13068] resolvePlugin reached for maven-clean-plugin" cannot pass without the extension being active and the default method being traversed. The test cannot false-pass.
Static analysis: ast-grep flagged a broad-exception catch in TestSuiteOrdering.java — pre-existing pattern, not introduced by this PR (the PR only adds a single addTestSuite(...) line). Semgrep: no findings.
Solid PR. The compat regression is real, the fix is minimal, and both the unit test (compile-time contract + reflection guard) and the IT (cross-version linkage + execution proof) are doing genuine work.
This review was generated by an AI agent, Hermès, on behalf of @gnodet.
Summary
Fixes #13068.
4.0.0-rc-6 added
resolveCoreExtensionAndFlattenandresolvePluginAndFlattento the internalPluginDependenciesResolveras abstract methods (#12335). Implementations that live out of tree — IntelliJ IDEA, Eclipse m2e, NetBeans all have one — are compiled against a different Maven than they run on, so the first plugin resolution dies withAbstractMethodError.This is the fix @gnodet announced for rc-7 in reply to the rc-6 vote.
Change
Both methods become
defaultand delegate to the pre-existingresolvePlugin, whichDefaultPluginDependenciesResolveralready treats as an alias — there,resolvePlugindelegates toresolvePluginAndFlatten.resolvePluginstays abstract on purpose: giving it a default too would let an implementation that overrides neither method recurse infinitely. There is an@implSpecnote warning against delegating back.The default for
resolveCoreExtensionAndFlattenis documented as best effort — a dedicated implementation additionally reads the extension's artifact descriptor to apply relocations and runMavenPluginDependenciesValidator. If you would rather not guess there, I am happy to make it throwUnsupportedOperationException; say the word.Tests
Unit —
PluginDependenciesResolverDefaultMethodsTest:resolvePluginwith the expected argumentsMethod.isDefault()guard, so re-abstracting either method fails the buildIntegration —
MavenITgh13068LegacyPluginDependenciesResolverTest: builds a core extension that overridesPluginDependenciesResolverwith only the pre-rc-6 method set, deliberately compiled againstmaven-core:4.0.0-rc-5, installs it, and runs a build that uses it via.mvn/extensions.xml. It asserts an error-free build and two marker lines, so it cannot pass without actually exercising the extension.Both were verified red without the change. Reverting only the interface gives:
Real-world verification
@chrisdutz confirmed that a
maven-4.0.xbuild carrying this patch imports Apache PLC4X in IntelliJ IDEA cleanly, where stock rc-6 produces a cascade of errors. PLC4X is affected viaresolveCoreExtensionAndFlattenbecause it declares core extensions in.mvn/extensions.xml.Worth noting the JetBrains side does not make this unnecessary:
resolveCoreExtensionAndFlattenis implemented only on their master and is absent from IntelliJ IDEA 2026.2.2, the current release.Note on the base branch
This targets
maven-4.0.xbecause that is where the regression shipped and where rc-7 / GA come from. Happy to open the equivalent againstmasterfirst if you prefer that order — the change applies unmodified, the baselines are identical.